Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise MapAt[] part 1 #1238

Merged
merged 3 commits into from
Dec 21, 2024
Merged

Revise MapAt[] part 1 #1238

merged 3 commits into from
Dec 21, 2024

Conversation

rocky
Copy link
Member

@rocky rocky commented Dec 21, 2024

Miscellaneous lint for expanding and correcting MapAt[]

  • add eval_MapAt (move out of builtin class), and start to address error messages more correctly
  • walk_parts -> eval_Part which is what it is. And move to mathics.eval.list.eol since that is where it belongs
  • DRY use of "psl" message
  • Drop "_" at the beginning of routines which weren't really internal.

@rocky rocky changed the title Revise map at Revise MapAt[] part 1 Dec 21, 2024
@rocky rocky requested a review from mmatera December 21, 2024 09:19
@rocky
Copy link
Member Author

rocky commented Dec 21, 2024

@mmatera I haven't begun to add the needed bug fixes for #1237 but this is already starting to get large.

@mmatera
Copy link
Contributor

mmatera commented Dec 21, 2024

@mmatera I haven't begun to add the needed bug fixes for #1237 but this is already starting to get large.

My only comment here is that walk_parts got its name from walk_levels, a routine already existing in the old Mathics code. That routine allowed both to find a part of a expression or to apply a function over some of the parts.

Copy link
Contributor

@mmatera mmatera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rocky
Copy link
Member Author

rocky commented Dec 21, 2024

@mmatera I haven't begun to add the needed bug fixes for #1237 but this is already starting to get large.

My only comment here is that walk_parts got its name from walk_levels, a routine already existing in the old Mathics code. That routine allowed both to find a part of a expression or to apply a function over some of the parts.

As the code is currently written, eval_Part exactly handles the Part[] built-in function when we don't have a ByteArray. Therefore it is exactly the kind of function that we have been calling eval_ and placing with a similar modular structure as builtin in mathics.eval.

Initially, my thought was that functions called eval_Xyy are likely to be the implementation of some future instruction set which would be used after compilation.

But what I am coming to appreciate in doing the debugger work is that by making things regular in this particular way, it makes things easier on a debugger or some other similar introspection tool. If I want to show, track, or stop at key parts of execution, looking for in routines in mathics_eval that have the name eval_Xyy (as opposed to eval_Xyy and walk_???) may be helpful.

And usually if I go up one level in a call stack often there will be an eval method which is also probably of interest.

Sure, I understand that a lot of what is there now may have some historical precedent. And the name walk_xxx conveys some sort of idea and possibly groups things along some lines.

In contrast to calling "elements", "leaves" or "eval" routines "apply", it is not a bad name or wrong, just not as helpful for the ways in which the code is currently envisioned to be organized in a way that makes it helpful to scale in the future.

It may very well be that in the future we may move and rename "walk_levels". For example that may also be the bulk of some builtin-function evaluation routine as well.

@rocky rocky merged commit b052251 into master Dec 21, 2024
14 checks passed
@rocky rocky deleted the revise-MapAt branch December 21, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants